Skip to content

Refactor WriterOptions processConfigs function#10762

Closed
kewang1024 wants to merge 1 commit intofacebookincubator:mainfrom
kewang1024:export-D61304850
Closed

Refactor WriterOptions processConfigs function#10762
kewang1024 wants to merge 1 commit intofacebookincubator:mainfrom
kewang1024:export-D61304850

Conversation

@kewang1024
Copy link
Copy Markdown
Contributor

Summary:
Merge processSessionConfigs and processHiveConnectorConfigs
as one function processConfigs.
In processConfigs(), it prioritize getting config from session property,
if not found, use config from connector config.

Differential Revision: D61304850

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 15, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 15, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 36d5f19
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66bfd7a41ccc5d00080c683c

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

Copy link
Copy Markdown
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kewang1024 thanks for working on this. Make sure the test all pass before land. Thanks!

Comment thread velox/dwio/dwrf/writer/Writer.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider to move the connector and session properties init into this function? Thanks!

@xiaoxmeng xiaoxmeng requested review from JkSelf and pedroerp August 15, 2024 19:25
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

1 similar comment
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

Summary:
Pull Request resolved: facebookincubator#10762

Merge processSessionConfigs and processHiveConnectorConfigs
as one function processConfigs.
In processConfigs(), it prioritize getting config from session property,
if not found, use config from connector config.

Reviewed By: xiaoxmeng

Differential Revision: D61304850
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61304850

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in 54b16e0.

@conbench-facebook
Copy link
Copy Markdown

Conbench analyzed the 1 benchmark run on commit 54b16e04.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants